feat(rules): compose multiple root rules safely - #2395
Conversation
rudironsoni
changed the title
rudironsoni
changed the title
dyoshikawa
left a comment
dyoshikawa
left a comment
There was a problem hiding this comment.
Before the code: #2393 was filed by the same account as this PR and nobody else has commented on it, so the acceptance criteria in it are your design rather than something we agreed on. Removing the one-root-per-target restriction is a deliberate guardrail that came out of #1247, and this PR also turns two previously-silent situations into hard failures. That is a product decision I need to make explicitly, so please treat the direction as open even though the implementation is solid. Happy to discuss it on the issue.
On the implementation itself, the core is genuinely good. I verified the premise (main really does throw on two matching root rules), and I looked hard at the test diff since that is where things usually go wrong: the six removed lines are the two rejects.toThrow assertions, and they were replaced with stronger positive assertions on count, bodies and order, not softened. The e2e matrix tests are untouched, the docs mirrors are in sync, and I confirmed that root composition cannot produce a file with frontmatter in the middle — every output that can land in a merge group is frontmatter-free, and every frontmatter-bearing output is isRoot() === false and therefore hits the reject path. Nice invariant.
The blocker is the case-only collision check. It is not scoped to groups that are actually merging, so it fires on ordinary unrelated rules and takes down the whole generate run. Details inline. Beyond that there is a trim regression for folding targets, an error message that cannot be traced back to source files, and a couple of smaller things.
One more that is not in the diff but becomes visible because of it: applyRootRuleSections injects the reference section and Additional Conventions into toolRules.find(rule => rule.isRoot()), i.e. only the first root. Fixed-root targets are fine since they end up with one file, but on cursor/augmentcode/takt with multiple surviving roots those sections now land in whichever rule sorts first by filename rather than in the overview. On cursor that can mean the memory-file references end up in a rule that is not always applied. Worth handling or at least documenting.
CI is green on all five jobs and I found nothing concerning security-wise: no network calls, no env/credential access, no CI or dependency changes.
|
Correction to my review: scratch the paragraph about It is latent rather than active, though: if a target ever emitted two The rest of the review stands. |
|
Thanks for the detailed review. I agree that the case-only check was too broad and that its policy had not been established. I removed it, so unrelated outputs such as I also restored trimming for singleton folding groups, separated the global composition coverage from the target matrix, documented the lexicographic source-path ordering, changed the helper to an object argument, and documented the single-surviving-root invariant in The focused processor suite passes 106 tests, the rules E2E suite passes 120 tests, and the full On the product decision, I agree that #2393 should not be treated as previously accepted direction. I do think multiple matching roots are the better supported behavior under this narrower policy. The restriction retained by #1247 validates source cardinality before target conversion. That was a useful guardrail when Rulesync could not distinguish whether two roots would conflict, but the target adapters already contain the information needed to make that decision at the actual output boundary. This change generalizes #1247's target-aware behavior into output-aware behavior:
This lets users split always-on instructions into independently maintained source files without forcing them to concatenate those sources manually for single-file tools. It also preserves each target's native model instead of imposing one source-file cardinality rule across every adapter. Ordering is deterministic and documented, and the collision rule now covers only the concrete content-loss case. With that narrowed scope, would you be comfortable treating multiple matching roots as supported behavior? |
8d4596a to
4ab1793
Compare
dyoshikawa
left a comment
dyoshikawa
left a comment
There was a problem hiding this comment.
Thanks for the rework — the case-only check is gone, the folding-target trim regression is fixed, and the error message now names the originating .rulesync/rules/*.md files. Those all look right, and I re-checked my own earlier claim about applyRootRuleSections: the "at most one root survives" comment does hold, since every rule class derives its root path from a fixed per-scope constant.
Two things still block this, both in mergeRulesByOutputPath.
The reject condition is !rootRule && !mergeNonRootRules, so a group that happens to contain a root rule is merged unconditionally, even on targets that cannot compose modular files. Kiro in global mode is the concrete case: the global root is .kiro/steering/product.md and non-root steering files land in the same directory, so a non-root rule named product.md gets concatenated onto the root and the generated file ends up with an inclusion: frontmatter block sitting in the middle of it. That is exactly the malformed output this PR sets out to prevent, and it is new — on main the same config produced one intact file via last-writer-wins. kiro-cli and kiro-ide share the class.
In the other direction the reject is too broad. Two non-root agentsmd rules sharing an agentsmd.subprojectPath produce plain frontmatter-less Markdown that composes perfectly well, but they now throw, and the message says the target "does not support composing modular rule files", which is not true for that target. The same applies to amp, factorydroid, kilo and opencode. And since generateRulesCore has no per-target try/catch, one devin kebab-case collision (CodingGuidelines.md + coding_guidelines.md — plausible in a real repo, and the shape your own test uses) fails the whole generate run including every other target and feature. Deciding by "does this output carry frontmatter" rather than "is there a root in the group" would fix both directions at once.
The new ordering contract in the docs also does not match loadRulesyncFiles, which concatenates [...discoveredFiles, ...discoveredCuratedFiles] — see the inline notes.
Smaller things, no need to block on them but worth a pass: it.each(["devin", "antigravity-ide"] as const) uses $toolTarget in the title, which only expands for object arrays, so both cases print the same name; the four new tests around lines 1287-1443 sit under the global-mode describe but do not pass global; and the group.length === 1 branch produces the same result as the general merge path below it, so it can collapse to just the !mergeNonRootRules early-continue.
One process note rather than a code one: #2393 is your own issue, so the "allow multiple roots" direction has not actually been agreed on our side yet. I'm fine with it in principle given the fail-closed framing, but it is a behavior change for existing configs that previously generated silently, so it needs a release-note callout.
4ab1793 to
d7fd3b3
Compare
|
@dyoshikawa do not merge, let's test this thoroughly and assert the intended product decision first! |
dyoshikawa
left a comment
dyoshikawa
left a comment
There was a problem hiding this comment.
I ran another pass over this, and I want to answer your "assert the intended product decision first" directly rather than merge it.
The composition half is sound. I re-derived the invariant your new comment at applyRootRuleSections claims and it holds across every adapter: buildToolRuleParamsDefault always routes root: true to the target's fixed root path, so a target can only ever have one root group, and every frontmatter-emitting adapter writes plain Markdown on its root branch — so concatenating root fragments can't inject a --- block mid-body. Ordering is genuinely deterministic (findFilesByGlobs sorts, then local before .curated/), which matches what you wrote in the docs. And I checked the test diff against main: the only two assertions you removed are the ones for the error this PR intentionally retires, and their replacements assert more than the originals did. No silent weakening, nothing suspicious anywhere in the diff.
What I'm not ready to sign off on is the second half. The hard error for modular same-path collisions is a separate decision from "multiple roots may compose", and it's the part #2393 never asked for. Removing the old Multiple root rulesync rules found guard newly exposes exactly two cases (takt's takt.name override, global kiro's fixed product.md); the non-root collisions the new error covers were already reachable on main, where they warned-or-overwrote rather than aborted. So this PR is converting a pre-existing silent-overwrite path into a generate-stopping exception, for targets that had nothing to do with the root-composition feature. That's defensible — an error beats losing a rule file — but it's my call to make, not something to fold in, and right now it's coupled to the feature so I can't take one without the other. I'd rather see the collision policy split out into its own issue and PR.
Two more things worth settling before either half lands. The guard is case-sensitive, so the most likely real-world collision slips through it, and the new test pins that silence as spec — details on the line. And it runs mid-way through convertRulesyncFilesToToolFiles, before appendSeparateConventionsRule and the root-mirror pushes, so it doesn't actually cover everything that reaches the writer; a check over the final file list would be both simpler and complete, and would let the merge function stop throwing.
Smaller stuff: the new mergeRulesByOutputPath JSDoc drops the tool-specific rationale the old comment carried; .rulesync/rules/overview.md:2 still has the old "less than or equal to one file" wording that you updated in both file-formats.md copies, and that file is what people copy as a template; and the docs now promise local-before-.curated/ ordering with no test pinning it. Nothing blocking, but the overview.md one is a real inconsistency now.
So: holding as you asked. Once we've settled whether the collision error ships here or separately, the rest is close.
d7fd3b3 to
ee1a42a
Compare
|
Implemented the requested product-policy split in Unsafe same-path groups involving a source I also aligned the template, canonical docs, generated skill mirrors, PR body, test placement, five-adapter composition matrix, case-warning coverage, and local-before-curated ordering coverage. Validation after rebasing onto current
The broader decision to hard-fail ordinary modular collisions is intentionally deferred. |
dyoshikawa
left a comment
dyoshikawa
left a comment
There was a problem hiding this comment.
Thanks for the rework — I went back through all six points from the last round and they are genuinely addressed. The hard error is scoped to groups involving a source root: true rule now, so the devin/copilot/.curated/ collisions that used to abort warn and keep generating; the case-insensitive pass runs over the final file list after conventions, MCP files and the root mirrors, and the test asserts the warning instead of asserting silence; the two booleans collapsed into one collisionPolicy; the provenance Map is gone; the JSDoc carries the dcode/Warp rationale again; the composition case covers all five re-routed targets; and the project-mode cases moved out of the global describe.
On the product decision I was holding on: scoping the throw to root-involved collisions and warning for the rest is exactly where I said I wanted it, so I am no longer asking to split the collision policy into its own PR. Consider that settled.
I also re-derived the composition invariant rather than take it on trust — I swept every target in toolRuleFactories with two root: true rules, in both project and global scope, and checked each generated file for a frontmatter block appearing mid-body. Nothing malformed, nothing throws. That half is solid.
What blocks it is the branch being stale, not the design. This is 44 commits behind main (merge-base 04d8e5d) and GitHub reports it as conflicting. I merged current main into a scratch worktree — only the two file-formats.md copies conflicted, the source auto-merged cleanly — and one of the new compose targets breaks in exactly the way this PR sets out to prevent. Details on the line.
So: please rebase on main and re-check the amp case, with a regression test as part of the fix — pnpm cicheck does not catch it as things stand (tsc clean, all 7816 unit tests green on the merged tree). The .curated/ note is smaller and I am happy to take it separately if you prefer.
| extension: "md", | ||
| supportsGlobal: true, | ||
| ruleDiscoveryMode: "toon", | ||
| collisionPolicy: "compose", |
There was a problem hiding this comment.
This is the blocker, and it only appears after a rebase.
Since this branch's merge-base, main taught AmpRule.fromRulesyncRule to prepend a globs: YAML block to non-root files — Amp's native conditional-load gate, from #2410. So amp is no longer a plain-Markdown adapter, and compose ends up concatenating two frontmatter-bearing bodies.
I merged current main into a scratch worktree and ran it. Two non-root rules that both carry agentsmd: { subprojectPath: "packages/app" } and non-empty globs produce a single packages/app/AGENTS.md:
---
globs:
- packages/app/**/*.ts
---
# first
---
globs:
- packages/app/**/*.tsx
---
# second
That is the mid-body frontmatter this PR exists to prevent. Amp reads only the leading block, so the second rule's gate is silently dropped, its body gets loaded under the first rule's globs, and a stray --- block is left sitting in the text. On main today the same config produces two separate well-formed files and last-writer-wins on disk: one rule lost, but nothing corrupt. The rebase turns a lossy-but-valid outcome into a malformed one.
I swept every target in toolRuleFactories for this shape and amp is the only one affected — the other four compose targets and every fold/preserve target are fine. So either drop amp from compose, or make the compose path strip each fragment's frontmatter and re-emit one merged globs: gate. Either way it needs the test below.
| ); | ||
| }); | ||
|
|
||
| it.each(["agentsmd", "amp", "factorydroid", "kilo", "opencode"] as const)( |
There was a problem hiding this comment.
This is why CI stays green on the amp problem: none of the rules here set globs, so nothing emits frontmatter and the composed output is plain Markdown for all five targets.
Please add a case where both rules carry non-empty globs. On this branch as-is it passes for all five; after a rebase onto main it fails for amp, which is exactly the signal you want.
| }); | ||
|
|
||
| describe("loadRulesyncFiles with curated rules", () => { | ||
| it("should compose local root fragments before curated root fragments", async () => { |
There was a problem hiding this comment.
Not blocking — happy to take this in a follow-up.
.curated/ is where rulesync add/fetch/install drop rules pulled from other repositories, and the guard this PR removes was the thing that made a third-party root fragment landing in CLAUDE.md/AGENTS.md impossible to miss: it aborted the run. Now it composes with no output at all. Root files are loaded unconditionally by every agent, so silently promoting remote content into one is worth a logger.warn when a .curated/ fragment is merged into a root output. formatRulePaths already gives you the wording.
| ... | ||
| ``` | ||
|
|
||
| Multiple files can set `root: true` for the same target in project and global modes. Rulesync renders each file through the target adapter, then combines compatible root or plain-Markdown single-file outputs in deterministic source-discovery order with one blank line between fragments. Local rules are ordered lexicographically by source file path and composed before non-overridden `.curated/` rules, which are also ordered lexicographically; filename prefixes such as `10-` and `20-` control composition order within each set. Targets that map source rules to distinct native paths keep those files separate. Explicitly supported plain-Markdown modular rules that normalize to the same output path are combined. Unsafe collisions involving a source `root: true` rule fail. Other exact or case-insensitive modular collisions remain separate and produce a warning that the last write wins wherever the filesystem treats their paths as the same. |
There was a problem hiding this comment.
"Explicitly supported plain-Markdown modular rules" stops being accurate for amp once you rebase — see the collisionPolicy comment. Whichever way you resolve that, this sentence and its skills/rulesync/file-formats.md mirror need to follow.
dyoshikawa
left a comment
dyoshikawa
left a comment
There was a problem hiding this comment.
Two more from a second pass, both smaller than the amp one but worth folding into the same round.
| const shouldCompose = | ||
| collisionPolicy === "fold" || collisionPolicy === "compose" || allGeneratedRulesAreRoots; | ||
|
|
||
| if (!shouldCompose && hasSourceRoot) { |
There was a problem hiding this comment.
This throw and the amp problem share a root cause worth naming: the decision is structural (what policy the target carries) rather than content-based (whether the rendered bodies actually contain a --- block). That cuts both ways.
Too strict here: kiro in global mode with a non-root product.md that has no globs renders as plain Markdown — no inclusion: block, nothing to corrupt — and it still aborts with "cannot safely compose", which isn't true for that input. Too loose over at collisionPolicy: "compose", which is how amp slipped through the moment #2410 gave it frontmatter. A content check (skip composing any fragment whose rendered body starts with ---) would fix both at once, and would not silently break again the next time one of agentsmd/factorydroid/kilo/opencode gains a frontmatter branch.
One knock-on effect to weigh: generateRulesCore in src/lib/generate.ts has no try/catch around its per-target loop — unlike generateIgnoreCore, which does. So this exception aborts rules generation for every remaining target, after the earlier ones have already been written. The old Multiple root rulesync rules found guard had the same property, but it never fired for the kiro-global shape, so for those users this turns a working (if lossy) generate into a hard stop with no way forward but renaming a file. At minimum the message should say that.
| if (previous) { | ||
| const previousPath = join(previous.getRelativeDirPath(), previous.getRelativeFilePath()); | ||
| this.logger.warn( | ||
| `Both ${describeSource(previous)} and ${describeSource(file)} generate to '${previousPath}' and '${path}' (compared case-insensitively, as on macOS and Windows); the last one wins wherever they collide.`, |
There was a problem hiding this comment.
For the common case — an exact path collision, not a case-only one — previousPath and path are the same string, so this reads "generate to '.devin/rules/coding-guidelines.md' and '.devin/rules/coding-guidelines.md'". Worth branching on previousPath === path and printing one path there; the import site's import to <path> phrasing is a good model.
Separately, describeSource falls back to the output path for files that aren't in convertedRules — the conventions rule, MCP instruction files, root mirrors — so a single message can end up mixing source paths and output paths. Minor, but it makes the warning harder to act on.
# Conflicts: # docs/reference/file-formats.md # skills/rulesync/file-formats.md
Since dyoshikawa#2410, Amp prepends a globs: frontmatter gate to non-root files, so composing two gated fragments buried the second gate mid-body where Amp never reads it. Composition safety is now decided by content: a colliding group only composes when every rendered fragment is plain Markdown; otherwise it falls back to preserve-and-warn, or fails when a source root rule is involved. Exact-path collision warnings also stop repeating the same path twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@rudironsoni I've pushed two commits here (using the maintainer-edit permission) so this can land without another round trip — the design was in good shape and I didn't want to hold it on a staleness issue that grew while the review was running. The merge commit brings the branch up to date with main; only the two The fix commit addresses the amp interaction I flagged: since #2410, amp prepends a Everything else from the review stands as written by you. Merging once CI is green. |
|
@rudironsoni Thank you! |
Summary
root: truerules in project and global modesroot: truerule before independently rendered metadata can be corruptedBehavior change
Multiple root fragments targeting the same compatible output are composed with a blank line between fragments. Explicitly supported plain-Markdown modular outputs are also composed. Unsafe same-path collisions involving a source
root: truerule fail before writing and report the generated path and originating.rulesync/rules/*.mdfiles.Pre-existing collisions between non-root modular rules remain backward compatible: Rulesync retains both generated outputs and warns that the last write wins wherever exact or case-insensitive paths collide.
Test plan
mise exec -- pnpm exec vitest run src/features/rules/rules-processor.test.ts --silent=falseNODE_ENV=e2e mise exec -- pnpm exec vitest run --config vitest.e2e.config.ts src/e2e/e2e-rules.spec.ts --silent=falsemise exec -- pnpm cicheckgit diff --checkCloses #2393